pkg/signature/sign_test.go (1)

170-175: Test case name is misleading - profile has empty annotations, not nil.

The test case "No annotations" uses NewMockSignableProfile which initializes annotations: make(map[string]string). This creates an empty map, not nil. The test likely passes because DecodeSignatureFromAnnotations fails on an empty map, but the test name suggests it's testing the nil check path.

Consider either:

1. Renaming to "Empty annotations", or
2. Using &MockSignableProfile{annotations: nil, ...} to truly test the nil case

📝 Suggested clarification

 		{
-			name:      "No annotations",
-			profile:   NewMockSignableProfile("uid", "ns", "name", map[string]string{}),
+			name:      "Nil annotations",
+			profile:   &MockSignableProfile{uid: "uid", namespace: "ns", name: "name", content: map[string]string{}, annotations: nil},
 			wantErr:   true,
 			setupSign: false,
 		},

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/sign_test.go` around lines 170 - 175, The test case name "No
annotations" is misleading because NewMockSignableProfile creates an empty map
(annotations: make(map[string]string)); update the test to either rename the
case to "Empty annotations" to reflect the actual input, or change the fixture
to construct a MockSignableProfile with annotations set to nil (e.g.,
&MockSignableProfile{annotations: nil, ...}) so it exercises the nil-path in
DecodeSignatureFromAnnotations; modify the entry that currently calls
NewMockSignableProfile("uid","ns","name", map[string]string{}) to your chosen
fix and update the test name accordingly.

pkg/signature/verify.go (1)

29-49: Consider adding a comment explaining why useKeyless: true is hardcoded for verification.

Both NewCosignAdapter(true) and NewCosignVerifier(true) use hardcoded true. This works because the verification uses the certificate stored in the annotations (from either keyless or key-based signing), but the reasoning isn't immediately obvious. A brief comment would improve clarity.

📝 Suggested comment

+	// useKeyless=true is fine for verification since we use the certificate
+	// stored in the profile annotations, regardless of how the profile was signed
 	adapter, err := NewCosignAdapter(true)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/verify.go` around lines 29 - 49, Add a short inline comment
next to the hardcoded useKeyless=true calls to NewCosignAdapter(true) and
NewCosignVerifier(true) explaining that verification uses the certificate
embedded in the annotations (so keyless=true is safe/intentional here), and
mention that signatures may come from either keyless or key-based signing but
the certificate in annotations is used for verification; place this comment
adjacent to the calls in VerifySignature (or the function containing adapter/
verifier creation) so future readers understand the rationale.

pkg/objectcache/applicationprofilecache/applicationprofilecache.go (1)

249-264: Consider extracting verification logic into a helper method to reduce duplication.

The same verification pattern is repeated three times (lines 249-264, 336-349, and 567-587). This could be extracted into a helper like verifyProfileIfEnabled(profile *v1beta1.ApplicationProfile, name, namespace string) error.

♻️ Proposed helper method

+// verifyApplicationProfile verifies the profile signature if verification is enabled.
+// Returns error if verification fails, nil otherwise (including when verification is disabled).
+func (apc *ApplicationProfileCacheImpl) verifyApplicationProfile(profile *v1beta1.ApplicationProfile, context string) error {
+	if !apc.cfg.EnableProfileVerification {
+		return nil
+	}
+	profileAdapter := profiles.NewApplicationProfileAdapter(profile)
+	if err := signature.VerifyProfile(profileAdapter); err != nil {
+		logger.L().Warning(context+" verification failed, skipping",
+			helpers.String("profile", profile.Name),
+			helpers.String("namespace", profile.Namespace),
+			helpers.Error(err))
+		return err
+	}
+	logger.L().Debug(context+" verification successful",
+		helpers.String("profile", profile.Name),
+		helpers.String("namespace", profile.Namespace))
+	return nil
+}

Then replace each verification block with:

if err := apc.verifyApplicationProfile(fullProfile, "profile"); err != nil {
    continue // or return, depending on context
}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/objectcache/applicationprofilecache/applicationprofilecache.go` around
lines 249 - 264, Extract the repeated signature verification block into a method
on applicationProfileCache (e.g., apc.verifyApplicationProfile) that checks
apc.cfg.EnableProfileVerification, constructs
profiles.NewApplicationProfileAdapter(fullProfile), calls
signature.VerifyProfile, logs the same Warning (including profile.Name,
namespace, workloadID when available) and Debug messages, and returns an error
when verification fails; then replace the three inline blocks that use
fullProfile/profile.Name/namespace/workloadID and signature.VerifyProfile with a
single call to apc.verifyApplicationProfile(...) and act on its error (continue
or return) as before.

pkg/signature/verify_test.go (1)

171-177: Test assertions are too weak - they only log but don't verify expected behavior.

The test checks if issuers/identities are the same and logs, but doesn't assert the expected difference between key-based (local) and keyless signing. Based on other tests, SignProfileWithKey should produce issuer local, while SignProfileKeyless produces a different issuer.

💚 Proposed fix with meaningful assertions

-	if sig1.Issuer == sig2.Issuer && sig1.Issuer != "" {
-		t.Log("Both profiles have same issuer")
-	}
-
-	if sig1.Identity == sig2.Identity && sig1.Identity != "" {
-		t.Log("Both profiles have same identity")
-	}
+	// Key-based signing should have "local" issuer
+	if sig1.Issuer != "local" {
+		t.Errorf("Expected key-based signing issuer 'local', got '%s'", sig1.Issuer)
+	}
+
+	// Keyless signing should have a non-local issuer (or at least be different in test env)
+	// Note: In test environment, keyless may also use "local" - adjust assertion as needed
+	if sig1.Identity != "local-key" {
+		t.Errorf("Expected key-based signing identity 'local-key', got '%s'", sig1.Identity)
+	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/verify_test.go` around lines 171 - 177, The test currently only
logs when sig1.Issuer/sig1.Identity equals sig2.*; replace those logs with
concrete assertions: verify that the profile signed with SignProfileWithKey
(sig1) has Issuer == "local" and that the profile signed with SignProfileKeyless
(sig2) does not have Issuer == "local" (use t.Fatalf/t.Errorf to fail the test
on mismatch), and similarly assert expected differences for Identity (e.g.,
assert sig1.Identity is set to the local identity and sig2.Identity differs or
is empty per the keyless behavior). Update the checks referencing sig1, sig2,
Issuer, Identity, SignProfileWithKey, and SignProfileKeyless to assert
equality/inequality rather than just t.Log.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

cmd/sign-profile/main.go (1)

48-53: ⚠️ Potential issue | 🔴 Critical

Default sign-profile --... still drops the first flag.

Line 87 always parses os.Args[2:]. When command is inferred (Line 43 path), the first real flag is at os.Args[1], so it gets skipped.

🐛 Proposed fix

-func parseSignFlags() {
+func parseSignFlags(args []string) {
 	fs := flag.NewFlagSet("sign-profile sign", flag.ExitOnError)
@@
-	if err := fs.Parse(os.Args[2:]); err != nil {
+	if err := fs.Parse(args); err != nil {
 		fmt.Fprintf(os.Stderr, "Error parsing flags: %v\n", err)
 		os.Exit(1)
 	}
@@
 	switch command {
 	case "sign", "":
-		parseSignFlags()
-		if argsRewritten {
-			os.Args = append([]string{"sign-profile"}, os.Args[1:]...)
-		}
+		if argsRewritten {
+			parseSignFlags(os.Args[1:])
+		} else {
+			parseSignFlags(os.Args[2:])
+		}

Also applies to: 78-88

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/sign-profile/main.go` around lines 48 - 53, The bug is that when command
is inferred (switch case "sign",""), parseSignFlags() is called while os.Args
still contains the original argv, then later code unshifts "sign-profile"
(argsRewritten path) causing the first real flag to be dropped because parsing
uses os.Args[2:]; fix by ensuring the argv mutation happens before parsing or by
parsing the correct slice based on argsRewritten: move the os.Args =
append([]string{"sign-profile"}, os.Args[1:]...) line to occur before calling
parseSignFlags(), or change parseSignFlags() invocation to pass the correct args
(use os.Args[1:] when argsRewritten is false and os.Args[2:] when a command
token is present) so that parseSignFlags() sees the first flag whether command
was explicit or inferred; update references to parseSignFlags, argsRewritten,
and the switch handling accordingly.

docs/signing/README.md (1)

450-459: ⚠️ Potential issue | 🟡 Minor

Fix ordered-list numbering continuity in Best Practices.

Line 450 starts another 3. after Line 445 already used 3.. Please renumber this sequence to keep it consistent.

📝 Suggested fix

-3. **Version Your Profiles**
+4. **Version Your Profiles**
    - Include version in metadata
    - Old signatures become invalid on content changes
 
-4. **Key Management for Local Signing**
+5. **Key Management for Local Signing**
    - Store keys in secure locations (HSM, KMS)
    - Rotate keys regularly
    - Use read-only keys for verification
 
-5. **Audit Trail**
+6. **Audit Trail**
    - Store signing timestamps
    - Track who signed what
    - Use GitHub Actions for audit logs

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/signing/README.md` around lines 450 - 459, The ordered list numbering in
the "Best Practices" section of docs/signing/README.md is inconsistent—after the
first "3." the subsequent list item "3. **Version Your Profiles**" should be
renumbered to continue the sequence; update the numeric markers for "Version
Your Profiles", "Key Management for Local Signing", and "Audit Trail" so they
follow the previous item (e.g., 4., 5., 6.) to restore correct ordered-list
continuity.

pkg/signature/cosign_adapter.go (1)

202-227: ⚠️ Potential issue | 🔴 Critical

Strict verification still accepts attacker-controlled certificate material.

In Line 215-223, strict mode only performs local certificate sanity checks (CA/CN/time) and does not verify certificate trust chains. Line 253-255 also treats sig.Issuer/sig.Identity as trusted if merely non-empty. This still allows self-consistent forged profile+cert+signature bundles to pass.

🔧 Suggested direction (trust-anchor + claim binding)

@@
 	if !allowUntrusted {
-		if cert.IsCA || cert.Subject.CommonName == "" {
-			return fmt.Errorf("invalid certificate: must not be CA and must have a valid subject")
-		}
-
-		if time.Now().Before(cert.NotBefore) || time.Now().After(cert.NotAfter) {
-			return fmt.Errorf("certificate is not valid at this time")
-		}
+		roots, err := x509.SystemCertPool() // preferably configured Fulcio/pinned roots
+		if err != nil || roots == nil {
+			return fmt.Errorf("failed to load trust roots: %w", err)
+		}
+		if _, err := cert.Verify(x509.VerifyOptions{
+			Roots:       roots,
+			CurrentTime: time.Now(),
+			KeyUsages:   []x509.ExtKeyUsage{x509.ExtKeyUsageCodeSigning},
+		}); err != nil {
+			return fmt.Errorf("certificate chain verification failed: %w", err)
+		}
+
+		if c.useKeyless {
+			if sig.Identity == "" || cert.Subject.CommonName != sig.Identity {
+				return fmt.Errorf("identity mismatch between signature metadata and certificate")
+			}
+			if sig.Issuer == "" || cert.Issuer.CommonName != sig.Issuer {
+				return fmt.Errorf("issuer mismatch between signature metadata and certificate")
+			}
+		}
 	}

Also applies to: 252-256

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/cosign_adapter.go` around lines 202 - 227, The current strict
path parses sig.Certificate but only runs local sanity checks (IsCA, CommonName,
dates) and later trusts sig.Issuer/sig.Identity if non-empty; fix by performing
a full x509 chain verification against a configured set of trust anchors and
intermediates (use x509.Verify with proper Roots and Intermediates and time
check) inside the block that handles sig.Certificate when allowUntrusted is
false, and require that x509.Verify succeeds; additionally, do not accept
sig.Issuer or sig.Identity as authoritative unless they are derived from the
verified certificate (e.g., compare sig.Issuer/sig.Identity to
cert.Subject/subjectAltName or parsed OID claims from cert) so only values bound
to a certificate that successfully verified are trusted.

pkg/objectcache/applicationprofilecache/applicationprofilecache.go (1)

252-258: Extract repeated “verification-failed” state updates into one helper.

These three blocks duplicate the same state-shaping logic. Centralizing it will reduce drift and keep status semantics consistent across paths.

Also applies to: 358-365, 589-596

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/objectcache/applicationprofilecache/applicationprofilecache.go` around
lines 252 - 258, Multiple code paths repeat creating an objectcache.ProfileState
with Completion:"failed", Status:"verification-failed", Name:profile.Name and
Error:err and then calling apc.workloadIDToProfileState.Set(workloadID,
profileState); factor this into a single helper on the receiver (e.g. func (apc
*ApplicationProfileCache) setVerificationFailed(workloadID string, profile
*ProfileType, err error)) that constructs the ProfileState and calls
apc.workloadIDToProfileState.Set; replace the three duplicated blocks (the
occurrences around the current diff and at lines ~358-365 and ~589-596) with
calls to that helper to keep status shaping consistent.

pkg/signature/cosign_adapter.go (1)

314-319: Use strict integer parsing for timestamp annotations.

fmt.Sscanf in Line 316 can accept partial numeric prefixes (e.g., "123abc"). strconv.ParseInt is stricter for annotation validation.

🔧 Suggested refactor

 import (
@@
 	"math/big"
+	"strconv"
 	"time"
@@
 	if timestamp, ok := annotations[AnnotationTimestamp]; ok {
-		var ts int64
-		_, err = fmt.Sscanf(timestamp, "%d", &ts)
+		ts, err := strconv.ParseInt(timestamp, 10, 64)
 		if err != nil {
 			return nil, fmt.Errorf("failed to parse timestamp: %w", err)
 		}
 		sig.Timestamp = ts
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/cosign_adapter.go` around lines 314 - 319, The code currently
uses fmt.Sscanf to parse the AnnotationTimestamp value (see AnnotationTimestamp
handling and the local variable ts), which allows partial numeric parsing;
replace that with strconv.ParseInt on the timestamp string (use base 10 and
64-bit) and assign the parsed int64 to ts, returning a wrapped error if ParseInt
fails to ensure strict integer validation of the annotation.

Configuration used: defaults

Review profile: CHILL

Plan: Pro

pkg/signature/sign_test.go (1)

200-204: ⚠️ Potential issue | 🟡 Minor

Check signing setup error in test case.

The SignObjectKeyless call in the setup ignores the returned error. If signing fails during setup, the test will proceed with an unsigned profile, making downstream assertions misleading.

💚 Proposed fix

 		t.Run(tt.name, func(t *testing.T) {
 			if tt.setupSign {
-				SignObjectKeyless(tt.profile)
+				if err := SignObjectKeyless(tt.profile); err != nil {
+					t.Fatalf("SignObjectKeyless setup failed: %v", err)
+				}
 			} else if tt.setupAnnotations != nil {
 				tt.setupAnnotations(tt.profile)
 			}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/sign_test.go` around lines 200 - 204, The test setup is
ignoring the error returned by SignObjectKeyless, which can leave the profile
unsigned and cause misleading failures; update the setup branch where
SignObjectKeyless(tt.profile) is called to capture its error and fail the test
on error (e.g., use t.Fatalf or require.NoError) so that SignObjectKeyless and
any errors are asserted, similar to how tt.setupAnnotations is invoked for
setupAnnotations(tt.profile).

pkg/signature/cosign_adapter.go (1)

287-370: ⚠️ Potential issue | 🟠 Major

Verification relies on self-signed certificates without external trust anchors.

This continues the concern from previous reviews: the verification accepts certificates and public keys from the signature annotations without validating against Fulcio roots or Rekor transparency logs. An attacker who can modify the profile can also replace the signature and certificate with their own self-signed material.

The allowUntrusted flag provides some distinction, but even "strict" mode (allowUntrusted=false) only validates:

• Certificate is not CA (line 305)
• Certificate is within validity period (lines 309-311)
• Identity matches CN (lines 313-315)

It does not validate the certificate chain against Fulcio roots or verify Rekor bundle entries.

For production use with real security requirements, consider integrating with sigstore's verification infrastructure.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/cosign_adapter.go` around lines 287 - 370, The VerifyData
function accepts certificates/public keys from the signature payload and only
checks basic properties (IsCA, validity window, CN) controlled by
allowUntrusted, which leaves verification vulnerable to self-signed or
attacker-provided certs; update VerifyData to perform real chain and
transparency verification: when allowUntrusted is false and c.useKeyless (or
when a certificate is present) validate the certificate chain against Fulcio
root(s) and, if present, verify the Rekor bundle/entry (or fail if missing)
using sigstore/cosign utilities (e.g., call sigstore trust root verification and
cosign.VerifyBundle or equivalent), ensure you reference sig.Certificate,
sig.RekorBundle (or add it to Signature), and the VerifyData code paths that
call sigstore_signature.LoadVerifier so the verifier is only created after
successful chain + Rekor verification.

pkg/signature/profiles/seccompprofile_adapter.go (1)

40-59: Consider initializing nil Labels for consistent JSON serialization.

When s.profile.Labels is nil, the JSON output will include "labels": null, but when it's an empty map, it will be "labels": {}. This inconsistency between cluster-side and signing-side representations could cause hash mismatches during verification.

The ApplicationProfileAdapter has normalization logic for PolicyByRuleId maps; consider applying similar treatment here for Labels if consistency is required.

♻️ Optional fix for label consistency

 func (s *SeccompProfileAdapter) GetContent() interface{} {
 	apiVersion := s.profile.APIVersion
 	if apiVersion == "" {
 		apiVersion = "spdx.softwarecomposition.kubescape.io/v1beta1"
 	}
 	kind := s.profile.Kind
 	if kind == "" {
 		kind = "SeccompProfile"
 	}
+	labels := s.profile.Labels
+	if labels == nil {
+		labels = make(map[string]string)
+	}
 	return map[string]interface{}{
 		"apiVersion": apiVersion,
 		"kind":       kind,
 		"metadata": map[string]interface{}{
 			"name":      s.profile.Name,
 			"namespace": s.profile.Namespace,
-			"labels":    s.profile.Labels,
+			"labels":    labels,
 		},
 		"spec": s.profile.Spec,
 	}
 }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/profiles/seccompprofile_adapter.go` around lines 40 - 59, The
GetContent method in SeccompProfileAdapter returns s.profile.Labels as-is which
yields "labels": null when nil and causes inconsistent JSON/hashing; update
SeccompProfileAdapter.GetContent to normalize labels by replacing a nil
s.profile.Labels with an empty map before building the returned map (i.e.,
ensure labels is a non-nil map[string]string when constructing the "metadata"
entry) so the JSON always serializes to "{}" for empty labels.

pkg/signature/cosign_adapter.go (3)

34-43: Prefer blank identifier imports over unused variable declarations.

These variables are declared but never used, serving only to ensure the packages are linked. This is unconventional and may trigger linter warnings. Use blank identifier imports instead.

♻️ Proposed refactor

-var _ = cosign.Signature
-var _ = providers.Enabled
-var _ = bundle.RekorBundle{}
-var _ = api.CertificateRequest{}
-var _ = client.Rekor{}
-var _ = models.LogEntry{}
-var _ = fulcioroots.Get
-var _ = oauthflow.OIDConnect
-var _ = oauthflow.DefaultIDTokenGetter

If these imports are needed for side effects, use blank identifier imports in the import block:

import (
	_ "github.com/sigstore/cosign/v2/pkg/cosign"
	// ... etc
)

If they're placeholders for future use, consider removing them until needed.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/cosign_adapter.go` around lines 34 - 43, Replace the unused
variable declarations (cosign.Signature, providers.Enabled, bundle.RekorBundle,
api.CertificateRequest, client.Rekor, models.LogEntry, fulcioroots.Get,
oauthflow.OIDConnect, oauthflow.DefaultIDTokenGetter) with proper blank
identifier imports in the package import block (e.g., import _ "module/path")
for any packages required only for side effects, or remove these placeholder
declarations entirely if the packages are not needed; update the import list to
include the corresponding packages with blank identifier imports instead of
keeping the var _ = ... lines so linters no longer flag unused variables.

---

217-246: Self-signed certificate has a one-year validity period.

The generated certificate is valid for 365 days (line 229). For keyless signing, certificates are typically short-lived (minutes) to reduce the window of key compromise. For key-based signing intended for offline/air-gapped environments, a longer validity may be acceptable.

Ensure this aligns with your security model and consider making the validity period configurable.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/cosign_adapter.go` around lines 217 - 246, The
generateCertificate function currently sets a fixed 365-day validity; make the
certificate lifetime configurable to support short-lived (minutes) keyless certs
or longer offline certs: add a CertValidity (time.Duration) field or
configuration on the CosignAdapter and use it when computing NotAfter (replace
time.Now().Add(365*24*time.Hour) with time.Now().Add(c.CertValidity)), provide a
sensible default (e.g., 15m for keyless) when constructing CosignAdapter, and
ensure any callers or tests are updated to set/expect the configured duration.

---

416-436: Silent fallback from base64 to raw bytes may mask encoding issues.

When base64 decoding fails, the code silently falls back to treating the value as raw bytes (lines 420-421, 427-428, 435-436). This could mask legitimate encoding errors or allow malformed data to pass through.

Consider logging a warning when falling back, or failing explicitly if the annotation is expected to be base64-encoded.

♻️ Proposed fix to add logging on fallback

 	sig.Signature, err = base64.StdEncoding.DecodeString(signatureB64)
 	if err != nil {
-		// Try raw if base64 fails
+		// Try raw if base64 fails - this handles legacy or non-standard encodings
 		sig.Signature = []byte(signatureB64)
+		// Consider: logger.L().Debug("Signature annotation not base64 encoded, using raw bytes")
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/cosign_adapter.go` around lines 416 - 436, The code silently
falls back to raw bytes when base64 decoding fails for sig.Signature,
sig.Certificate and sig.RekorBundle (base64.StdEncoding.DecodeString used
against annotations[AnnotationCertificate] and
annotations[AnnotationRekorBundle]), which can hide encoding problems; update
each decode branch to emit a warning (e.g., logger.Warnf or similar) when
DecodeString returns an error that includes the annotation key and the decode
error, and only fallback to raw bytes if configured to be tolerant (or
alternatively return the error to the caller for strict mode); ensure the
warning references sig.Signature, sig.Certificate and sig.RekorBundle so it’s
clear which field had the issue.

pkg/signature/profiles/applicationprofile_adapter.go (2)

67-76: Consider initializing nil Labels for consistent JSON serialization.

Same concern as SeccompProfileAdapter: when a.profile.Labels is nil, JSON renders "labels": null vs "labels": {} for empty maps, potentially causing hash mismatches during verification.

♻️ Optional fix for label consistency

+	labels := a.profile.Labels
+	if labels == nil {
+		labels = make(map[string]string)
+	}
 	return map[string]interface{}{
 		"apiVersion": apiVersion,
 		"kind":       kind,
 		"metadata": map[string]interface{}{
 			"name":      a.profile.Name,
 			"namespace": a.profile.Namespace,
-			"labels":    a.profile.Labels,
+			"labels":    labels,
 		},
 		"spec": a.profile.Spec,
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/profiles/applicationprofile_adapter.go` around lines 67 - 76,
The returned manifest may emit "labels": null when a.profile.Labels is nil,
causing inconsistent JSON/hashing; update the ApplicationProfileAdapter code
that builds the map (the block returning apiVersion/kind/metadata/spec) to
ensure labels is always a non-nil map by replacing "labels": a.profile.Labels
with a guarded value that returns an empty map (e.g., map[string]string{}) when
a.profile.Labels == nil, otherwise use a.profile.Labels so serialization yields
{} instead of null.

---

43-57: Normalization mutates the original profile object.

The PolicyByRuleId normalization directly modifies a.profile.Spec.Containers[i].PolicyByRuleId. While this ensures consistent JSON representation for signing, it has the side effect of mutating the caller's profile object. If the caller expects the profile to remain unchanged after calling GetContent(), this could lead to unexpected behavior.

Consider whether this mutation is intentional or if a defensive copy should be made.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/profiles/applicationprofile_adapter.go` around lines 43 - 57,
The normalization loop in GetContent (which sets PolicyByRuleId on
a.profile.Spec.Containers / InitContainers / EphemeralContainers) mutates the
original profile; instead make a defensive deep copy of the profile/spec before
normalizing so callers' objects are unchanged. Update the code in
applicationprofile_adapter.go to copy a.profile (or at least a.profile.Spec and
its Containers/InitContainers/EphemeralContainers and their PolicyByRuleId maps)
into a local variable, perform the nil->make map initialization on that copy,
and then use the copy for JSON/signing output (avoid changing a.profile in
place). Ensure copies include container slices and nested maps so no original
reference is modified.

cmd/sign-object/main.go (1)

43-47: ⚠️ Potential issue | 🟡 Minor

Global help flags still get rewritten to sign.

Running sign-object -h or sign-object --help matches the strings.HasPrefix(command, "-") check before reaching the explicit help cases in the switch, causing users to see sign-specific help instead of global usage.

🐛 Proposed fix

 	command = os.Args[1]
 
 	argsRewritten := false
+	if command == "-h" || command == "--help" {
+		printUsage()
+		os.Exit(0)
+	}
 	if strings.HasPrefix(command, "-") {
 		command = "sign"
 		argsRewritten = true
 	}

,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmd/sign-object/main.go` around lines 43 - 47, The current rewrite logic sets
command = "sign" when strings.HasPrefix(command, "-"), which incorrectly catches
global help flags like "-h" and "--help"; change the condition so help flags are
exempt (e.g., check for "-h" and "--help" first or require
strings.HasPrefix(command, "-") && command != "-h" && command != "--help") so
argsRewritten is only set for real subcommand shortcuts while preserving the
explicit help cases in the existing switch that handle "-h"/"--help".

docs/signing/README.md (2)

72-104: ⚠️ Potential issue | 🟡 Minor

Add the missing opening mermaid fence.

This block still starts as plain text and only has the closing fence, which matches the markdownlint failures on Line 89 and Line 104.

Suggested fix

 ### Diagram
+```mermaid
 graph TB
     subgraph "Signing Flow"
         A[K8s Resource] --> B[Adapter]
@@
 
     L --> M

</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @docs/signing/README.md around lines 72 - 104, Add the missing opening fenced
code block marker for the Mermaid diagram by inserting an opening "mermaid" fence immediately before the "graph TB" line so the block is recognized as Mermaid (the block already contains the closing ""); ensure there is no extra
indentation and the opening fence matches the existing closing fence to resolve
the markdownlint errors around the diagram section.

</details>

---

`257-263`: _⚠️ Potential issue_ | _🟡 Minor_

**Fix the `generate-keypair` output description.**

The table still implies both keys are written into one PEM file. The CLI writes the private key to `--output` and the public key to `--output + ".pub"`; `--public-only` should say that only the public file is emitted.

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@docs/signing/README.md` around lines 257 - 263, Update the generate-keypair
flags documentation to accurately describe where each key is written: change the
`--output` description to state it writes the private key to the specified PEM
file and that the public key is written to `--output + ".pub"`, and change the
`--public-only` description to state it emits only the public key file (i.e.
only `--output + ".pub"`) rather than implying a combined PEM; reference the
`--output` and `--public-only` flags in the README table for these exact wording
changes.
```

</details>

</blockquote></details>

</blockquote></details>

<details>
<summary>🧹 Nitpick comments (2)</summary><blockquote>

<details>
<summary>cmd/sign-object/main.go (2)</summary><blockquote>

`52-63`: **Remove dead code: `os.Args` modifications after parsing.**

These `os.Args` assignments occur after `parse*Flags()` functions have already completed parsing. They have no effect on the program behavior.

<details>
<summary>♻️ Proposed cleanup</summary>

```diff
 	case "sign", "":
 		parseSignFlags()
-		if argsRewritten {
-			os.Args = append([]string{"sign-object"}, os.Args[1:]...)
-		}
 	case "verify":
 		parseVerifyFlags()
-		os.Args = append([]string{"sign-object verify"}, os.Args[2:]...)
 	case "generate-keypair":
 		parseGenerateFlags()
-		os.Args = append([]string{"sign-object generate-keypair"}, os.Args[2:]...)
 	case "extract-signature":
 		parseExtractFlags()
-		os.Args = append([]string{"sign-object extract-signature"}, os.Args[2:]...)
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@cmd/sign-object/main.go` around lines 52 - 63, Remove the dead os.Args
reassignments that occur after flag parsing: the append calls modifying os.Args
in the branches for "verify", "generate-keypair", and "extract-signature" (and
the argsRewritten handling) have no effect because parseVerifyFlags(),
parseGenerateFlags(), and parseExtractFlags() already completed parsing; delete
those os.Args = append(...) lines (and any unused argsRewritten-related branch
if it is now unused) to clean up dead code while keeping the parse*Flags() calls
intact.
```

</details>

---

`387-391`: **Certificate type display may silently fail for non-PEM certificates.**

The signature annotations store certificates as base64-encoded raw bytes. `pem.Decode` returns `nil` for raw DER-encoded certificates, silently skipping the certificate type display. Consider parsing as DER if PEM decode fails.

<details>
<summary>♻️ Suggested improvement</summary>

```diff
 		block, _ := pem.Decode(sig.Certificate)
 		if block != nil {
 			fmt.Printf("  Certificate Type: %s\n", block.Type)
+		} else if len(sig.Certificate) > 0 {
+			// Try parsing as DER
+			cert, err := x509.ParseCertificate(sig.Certificate)
+			if err == nil {
+				fmt.Printf("  Certificate Subject: %s\n", cert.Subject.CommonName)
+			}
 		}
```
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

```
Verify each finding against the current code and only fix it if needed.

In `@cmd/sign-object/main.go` around lines 387 - 391, The code currently uses
pem.Decode(sig.Certificate) and only prints block.Type when block != nil, which
silently skips DER/raw certificates; update the logic in the certificate
handling (around pem.Decode and printing of certificate type) to attempt DER
parsing when pem.Decode returns nil: first try pem.Decode(sig.Certificate) and
print block.Type if present, otherwise try parsing sig.Certificate as a DER x509
certificate (using x509.ParseCertificate) and on success print an appropriate
type string (e.g., "X.509 DER") or the certificate's Subject/Issuer; ensure
error handling/logging for failed parses so non-PEM certificates don't silently
skip type output.
```

</details>

</blockquote></details>

</blockquote></details>

<details>
<summary>🤖 Prompt for all review comments with AI agents</summary>

Verify each finding against the current code and only fix it if needed.

Inline comments:
In @pkg/objectcache/networkneighborhoodcache/networkneighborhoodcache.go:

• Around line 327-340: The current code verifies the post-merge object
  (mergedNN) using signature.VerifyObjectStrict after performMerge, but
  performMerge mutates content while retaining original annotations so
  verification will incorrectly fail for valid user overrides; instead, when
  nnc.cfg.EnableSignatureVerification is true, call signature.VerifyObjectStrict
  on the fetched source objects (originalNN and fullUserNN) before performMerge
  (using profiles.NewNetworkNeighborhoodAdapter on each) OR, if mergedNN must be
  cacheable, re-sign the merged result after performMerge (update mergedNN
  annotations and call the signing API) and then use VerifyObjectStrict on the
  re-signed mergedNN; update the code paths around performMerge,
  NewNetworkNeighborhoodAdapter, and VerifyObjectStrict accordingly so signature
  checks validate the unmodified source or a freshly-signed merged object.
• Around line 243-254: The signature-verification branch currently logs and
  continues without updating workloadIDToProfileState, causing callers of
  GetNetworkNeighborhoodState to miss the failure; modify the block inside
  nnc.cfg.EnableSignatureVerification (where adapter :=
  profiles.NewNetworkNeighborhoodAdapter(fullNN) and
  signature.VerifyObjectStrict(adapter) is called) to persist the verification
  failure into workloadIDToProfileState: create or update the ProfileState entry
  for workloadID (and namespace/name as appropriate) to mark it failed/errored and
  include the verification error (err.Error()) and any relevant metadata before
  continuing, so the state reflects the rejected profile.

---

Duplicate comments:
In @cmd/sign-object/main.go:

• Around line 43-47: The current rewrite logic sets command = "sign" when
  strings.HasPrefix(command, "-"), which incorrectly catches global help flags
  like "-h" and "--help"; change the condition so help flags are exempt (e.g.,
  check for "-h" and "--help" first or require strings.HasPrefix(command, "-") &&
  command != "-h" && command != "--help") so argsRewritten is only set for real
  subcommand shortcuts while preserving the explicit help cases in the existing
  switch that handle "-h"/"--help".

In @docs/signing/README.md:

• Around line 72-104: Add the missing opening fenced code block marker for the
  Mermaid diagram by inserting an opening "mermaid" fence immediately before the "graph TB" line so the block is recognized as Mermaid (the block already contains the closing ""); ensure there is no extra indentation and the
  opening fence matches the existing closing fence to resolve the markdownlint
  errors around the diagram section.
• Around line 257-263: Update the generate-keypair flags documentation to
  accurately describe where each key is written: change the --output description
  to state it writes the private key to the specified PEM file and that the public
  key is written to --output + ".pub", and change the --public-only
  description to state it emits only the public key file (i.e. only --output + ".pub") rather than implying a combined PEM; reference the --output and
  --public-only flags in the README table for these exact wording changes.

---

Nitpick comments:
In @cmd/sign-object/main.go:

• Around line 52-63: Remove the dead os.Args reassignments that occur after flag
  parsing: the append calls modifying os.Args in the branches for "verify",
  "generate-keypair", and "extract-signature" (and the argsRewritten handling)
  have no effect because parseVerifyFlags(), parseGenerateFlags(), and
  parseExtractFlags() already completed parsing; delete those os.Args =
  append(...) lines (and any unused argsRewritten-related branch if it is now
  unused) to clean up dead code while keeping the parse*Flags() calls intact.
• Around line 387-391: The code currently uses pem.Decode(sig.Certificate) and
  only prints block.Type when block != nil, which silently skips DER/raw
  certificates; update the logic in the certificate handling (around pem.Decode
  and printing of certificate type) to attempt DER parsing when pem.Decode returns
  nil: first try pem.Decode(sig.Certificate) and print block.Type if present,
  otherwise try parsing sig.Certificate as a DER x509 certificate (using
  x509.ParseCertificate) and on success print an appropriate type string (e.g.,
  "X.509 DER") or the certificate's Subject/Issuer; ensure error handling/logging
  for failed parses so non-PEM certificates don't silently skip type output.

</details>

---

<details>
<summary>ℹ️ Review info</summary>

<details>
<summary>⚙️ Run configuration</summary>

**Configuration used**: defaults

**Review profile**: CHILL

**Plan**: Pro

**Run ID**: `2d6ebf82-5539-4d16-9058-e53243fbe4fb`

</details>

<details>
<summary>📥 Commits</summary>

Reviewing files that changed from the base of the PR and between 411ed7b076e8d520b45ac3a6e8309898be137423 and 479285c088fb78f1e1af7ad599f91f87d4fc27c9.

</details>

<details>
<summary>📒 Files selected for processing (3)</summary>

* `cmd/sign-object/main.go`
* `docs/signing/README.md`
* `pkg/objectcache/networkneighborhoodcache/networkneighborhoodcache.go`

</details>

</details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

pkg/objectcache/networkneighborhoodcache/networkneighborhoodcache.go (1)

329-373: ⚠️ Potential issue | 🟠 Major

Drop stale signature annotations before caching the merged profile.

performMerge changes the signed content, but mergedNN still inherits the original signature metadata from DeepCopy(). That leaves a mutated object in cache that still looks signed, and any downstream re-verification will fail against stale annotations.

🧹 Proposed fix

 	// Merge the network neighborhoods
 	mergedNN := nnc.performMerge(originalNN, fullUserNN)
+	for _, key := range []string{
+		signature.AnnotationSignature,
+		signature.AnnotationCertificate,
+		signature.AnnotationRekorBundle,
+		signature.AnnotationIssuer,
+		signature.AnnotationIdentity,
+		signature.AnnotationTimestamp,
+	} {
+		delete(mergedNN.Annotations, key)
+	}
 
 	// Update the cache with the merged network neighborhood
 	nnc.workloadIDToNetworkNeighborhood.Set(toMerge.wlid, mergedNN)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/objectcache/networkneighborhoodcache/networkneighborhoodcache.go` around
lines 329 - 373, The merged network neighborhood returned by
nnc.performMerge(originalNN, fullUserNN) still carries signature-related
annotations from the DeepCopy of original objects; before calling
nnc.workloadIDToNetworkNeighborhood.Set(toMerge.wlid, mergedNN) remove/clear the
stale signature metadata (i.e., any annotation keys the
signature.VerifyObjectStrict/Sign logic uses) from mergedNN.Annotations so the
cached object no longer appears signed and won't trigger false re-verification
failures.

pkg/signature/cosign_adapter.go (1)

334-397: ⚠️ Potential issue | 🔴 Critical

allowUntrusted=false still trusts key material from the object itself.

The strict path accepts a raw PUBLIC KEY PEM from annotations, and even the CERTIFICATE branch never chains the leaf to a trusted Fulcio root or validates Rekor/CT material. A user who can rewrite the object can therefore replace both the content and verification key and still make VerifyObjectStrict pass. Sigstore's keyless verification requires the certificate chain plus Rekor/CT evidence, not just an embedded PEM blob. (docs.sigstore.dev)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/cosign_adapter.go` around lines 334 - 397, The current strict
verification path (in the pem.Decode branch handling "CERTIFICATE" and "PUBLIC
KEY" inside VerifyObjectStrict) accepts an embedded PUBLIC KEY PEM and does not
validate a certificate chain or Rekor/CT evidence, allowing object authors to
substitute keys; fix by forbidding or treating embedded public keys as untrusted
when allowUntrusted==false and by requiring and validating a proper x509 chain
and SignedEntryTimestamp/Rekor proof for Fulcio-issued certificates: parse the
certificate chain (not just the leaf) from sig.Certificate, verify it against
the Fulcio root set and system roots, validate certificate validity period and
key usage, and run Sigstore/Cosign Rekor/CT verification APIs (the library
verification that checks SignedEntryTimestamp/Rekor proof) before calling
sigstore_signature.LoadVerifier; ensure any fallback PUBLIC KEY handling is only
allowed when allowUntrusted==true.

pkg/signature/profiles/networkneighborhood_adapter_test.go (1)

11-65: Add coverage for the empty-TypeMeta path.

This test only exercises the branch where APIVersion and Kind are already populated. GetContent() has separate defaulting logic for empty TypeMeta, and those fields are part of the canonical hash used by signing/verification, so a regression there would not be caught here.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/profiles/networkneighborhood_adapter_test.go` around lines 11 -
65, Add a variant in TestNetworkNeighborhoodAdapter that constructs a
v1beta1.NetworkNeighborhood with an empty TypeMeta (no Kind/APIVersion), pass it
through NewNetworkNeighborhoodAdapter and call adapter.GetContent(), then assert
the returned content map contains the defaulted "kind" == "NetworkNeighborhood"
and "apiVersion" == "softwarecomposition.kubescape.io/v1beta1" (and validate
metadata.name/namespace and spec container content as in the existing
assertions); this ensures GetContent()'s defaulting path for empty TypeMeta is
covered and included in signing/verification hashing.

pkg/signature/cluster_flow_test.go (1)

104-112: Exercise the package-level annotation flow in this test.

The fixture hand-writes only signature and certificate annotations and then verifies with cert.PublicKey directly, so it won't catch regressions in EncodeSignatureToAnnotations, issuer/identity/timestamp handling, or VerifyObject*. Building the annotations via EncodeSignatureToAnnotations and asserting VerifyObjectAllowUntrusted(adapter) here would cover the flow the rest of the package actually uses.

Also applies to: 117-140

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/cluster_flow_test.go` around lines 104 - 112, The test
currently hand-sets "signature" and "certificate" annotations and decodes via
cosignAdapter.DecodeSignatureFromAnnotations; instead exercise the package-level
annotation flow by using cosignAdapter.EncodeSignatureToAnnotations (or
EncodeSignatureToAnnotations) to produce the annotations from sig and certBytes,
apply them to adapter via adapter.SetAnnotations, then call
cosignAdapter.VerifyObjectAllowUntrusted(adapter) (or
VerifyObjectAllowUntrusted) and assert no error; this ensures
EncodeSignatureToAnnotations, issuer/identity/timestamp handling, and
VerifyObject* are validated rather than bypassed.

pkg/signature/cosign_adapter.go (3)

343-397: ⚠️ Potential issue | 🔴 Critical

allowUntrusted=false still accepts attacker-supplied leaf certs.

The strict path parses the certificate bundled in the annotations and immediately uses cert.PublicKey, but it never verifies that certificate against any configured trust root or pinned key set. A modified object can be re-signed with a fresh self-signed leaf cert and still pass "strict" verification.

#!/bin/bash
set -euo pipefail

sed -n '343,397p' pkg/signature/cosign_adapter.go
printf '\n=== certificate-chain verification hooks ===\n'
rg -n 'cert\.Verify|x509\.VerifyOptions|x509\.CertPool|fulcioroots\.Get|VerifySignedCertificateTimestamp' pkg/signature/cosign_adapter.go

Expected result: the strict path loads cert.PublicKey directly and does not perform certificate-chain/root validation before trusting it.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/cosign_adapter.go` around lines 343 - 397, The strict path
currently trusts the leaf certificate in the annotation (used when
allowUntrusted is false) without performing certificate-chain or root
verification; update the code around the block that calls
sigstore_signature.LoadVerifier(cert.PublicKey, crypto.SHA256) (and the
alternative public-key path using cryptoutils.UnmarshalPEMToPublicKey) to first
build and verify the x509 chain: create x509.VerifyOptions with a root pool
(e.g., fulcioroots.Get or configured CertPool), any intermediate pool, and
appropriate DNS/Time checks, call cert.Verify(VerifyOptions), and only on
successful verification proceed to LoadVerifier; if verification fails return an
explicit error about untrusted certificate chain.

---

348-350: ⚠️ Potential issue | 🟠 Major

Validate certificate lifetime at signing time, not wall-clock time.

Using time.Now() makes valid keyless signatures start failing as soon as the short-lived Fulcio cert expires. Compare sig.Timestamp to NotBefore/NotAfter so historical verification still works.

Proposed fix

-				if time.Now().Before(cert.NotBefore) || time.Now().After(cert.NotAfter) {
+				signedAt := time.Unix(sig.Timestamp, 0)
+				if signedAt.Before(cert.NotBefore) || signedAt.After(cert.NotAfter) {
 					return fmt.Errorf("certificate is not valid at this time")
 				}

When verifying Sigstore/Fulcio keyless signatures, should certificate validity be checked against the signing timestamp instead of the verifier's current wall-clock time?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/cosign_adapter.go` around lines 348 - 350, The certificate
validity check uses time.Now(); change it to validate against the signature's
signing time by comparing cert.NotBefore and cert.NotAfter to the signature
timestamp (sig.Timestamp) instead of wall-clock time. Convert sig.Timestamp into
a time.Time (e.g., time.Unix(sig.Timestamp, 0) or use the existing timestamp
type) and replace the two time.Now().Before/After comparisons with comparisons
against that signing time so historical verifications succeed; update the
condition around cert, sig.Timestamp, and the error return accordingly.

---

129-150: ⚠️ Potential issue | 🟠 Major

Keyless signing can't depend on browser auth or hard-coded claims.

The provider branch records fixed identity/issuer values, and the fallback opens an interactive OIDC flow from library code. That's already breaking CI, and it decouples the stored signer metadata from the actual token that Fulcio authenticated. Make the token source injectable/non-interactive and derive identity/issuer from the token claims.

How does Sigstore/Fulcio keyless signing derive signer identity and issuer from an OIDC token, and is it valid to hard-code those fields or require an interactive browser fallback in headless CI?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/cosign_adapter.go` around lines 129 - 150, The code currently
uses providers.Enabled/Provide to get tok but then hard-codes identity/issuer to
sigstoreOIDC/sigstoreIssuer and falls back to oauthflow.OIDConnect (interactive)
when no provider is enabled; replace this by making the token source injectable
(accept a non-interactive token provider or token string from context/params
instead of calling oauthflow.OIDConnect), stop using sigstoreOIDC/sigstoreIssuer
constants, and parse/validate the returned OIDC token (the tok value) to extract
claims for Subject and Issuer to populate identity and issuer; ensure functions
that call this code (or the constructor) accept a TokenProvider or raw token so
CI/headless environments can supply tokens without browser auth and the signer
metadata is derived from the token claims rather than hard-coded values.

pkg/signature/sign.go (1)

77-79: SignObjectWithKey is misleading.

This helper only disables keyless mode; when no WithPrivateKey option is passed, NewCosignAdapter(false) generates a new keypair. For an offline-signing API, either accept a private key here or rename the helper to match what it actually does.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/sign.go` around lines 77 - 79, The helper SignObjectWithKey is
misleading because it only sets WithKeyless(false) and still allows
NewCosignAdapter(false) to generate a new keypair; fix by changing the helper to
accept a private key parameter and pass it through to SignObject, e.g. change
func SignObjectWithKey(obj SignableObject) to func SignObjectWithKey(obj
SignableObject, privateKey string) error and call SignObject(obj,
WithKeyless(false), WithPrivateKey(privateKey)); alternatively, if you prefer
not to change the API, rename the function to reflect that it only disables
keyless mode (e.g., SignObjectDisableKeyless) and update all callers and tests
to match the new signature/name.

pkg/signature/cosign_adapter.go (2)

331-340: ⚠️ Potential issue | 🔴 Critical

Guard VerifyData against nil sig before dereferencing it.

A nil sig panics at Line 339 (pem.Decode(sig.Certificate)) instead of failing closed. That turns malformed decode paths into a crash in the verification pipeline.

🐛 Proposed fix

 func (c *CosignAdapter) VerifyData(data []byte, sig *Signature, allowUntrusted bool) error {
+	if sig == nil {
+		return fmt.Errorf("VerifyData: signature is nil")
+	}
+
 	var verifier sigstore_signature.Verifier
 	var err error

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/cosign_adapter.go` around lines 331 - 340, The VerifyData
function risks a panic when sig is nil because it dereferences sig to call
pem.Decode(sig.Certificate); add a nil-check at the start of VerifyData (in the
CosignAdapter.VerifyData function) to return a clear error if sig == nil before
any use of sig, and ensure downstream code (including the pem.Decode call and
any uses of sig.Certificate or sig.PublicKey) only executes when sig is non-nil.

---

129-141: ⚠️ Potential issue | 🟠 Major

Derive the signer identity from the returned OIDC token, not constants.

Lines 140-141 still hard-code sigstoreOIDC / sigstoreIssuer after providers.Provide(...). Fulcio binds the certificate to the token’s real claims, and VerifyData later matches sig.Identity against that certificate. This can stamp incorrect metadata and make provider-path signatures fail verification for any non-matching subject/issuer.

For Sigstore keyless signing with github.com/sigstore/cosign/v3/pkg/providers, should the signer identity and issuer be derived from the returned OIDC token claims instead of hard-coding them? Please point to the official Sigstore/Fulcio guidance.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/signature/cosign_adapter.go` around lines 129 - 141, After calling
providers.Provide(ctx, "sigstore") you must extract the actual OIDC token claims
from the returned token and set identity and issuer from those claims instead of
assigning the constants sigstoreOIDC/sigstoreIssuer; update the logic around
providers.Provide -> tok (or the returned token structure) to decode the token
claims (subject/sub and issuer/iss) and assign them to identity and issuer so
that VerifyData and sig.Identity will match the Fulcio-bound certificate; modify
the code paths that currently reference sigstoreOIDC and sigstoreIssuer to use
the parsed claims from the token returned by providers.Provide and ensure any
JWT/ID token parsing uses the token type returned by providers.Provide (or a
simple JWT parser if necessary) to extract "sub" and "iss".